Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement pallet_xcm_core_buyer #452

Merged
merged 23 commits into from
Mar 22, 2024
Merged

Implement pallet_xcm_core_buyer #452

merged 23 commits into from
Mar 22, 2024

Conversation

tmpolaczyk
Copy link
Contributor

@tmpolaczyk tmpolaczyk commented Mar 12, 2024

This pallet allows collators to buy parathread cores on demand.

It uses XCM to send a message to the relay chain, to the extrinsic OnDemandAssignmentProvider.place_order_allow_death. The core and the fees are paid using the parathread tank account in the relay chain, which is an account derived through XCM where parathread managers are supposed to send DOT tokens to.

It provides 3 extrinsics:

  • buy_core

(won't be implemented in this PR) Any collators that are currently assigned to a parathread can call this extrinsic to send an XCM message to the relay chain. We use an ensure_none origin to allow collators to call this extrinsic without paying any fees by providing a proof (proof validation is not implemented, so currently all calls will fail). Only 1 XCM message is allowed per tanssi block per parathread.

  • force_buy_core

Same as buy_core but can only be called by root. Instead of a proof, we simply check that at least one collator is assigned to the para_id. We need this check because it doesn't make sense to buy a core for a parathread that doesn't have any assigned collators, because nobody will produce the block. This is mainly used in tests because the other extrinsic is not implemented yet.

  • set_xcm_weights

Only callable by root, this is used to set a storage item which contains the withdraw amount needed to BuyExecution, and the weight_at_most of the place_order_allow_death call. This is a storage item because relay chain weights can change, so we need to be able to adjust them without doing a runtime upgrade.

WIP

Update polkadot-sdk, include cherry-pick DescribeAllTerminal
@girazoki girazoki requested review from ParthDesai and girazoki March 13, 2024 08:30
@tmpolaczyk tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Mar 13, 2024
Copy link
Contributor

github-actions bot commented Mar 14, 2024

Coverage Report

(master)

@@                   Coverage Diff                   @@
##           master   tomasz-xcm-coretime      +/-   ##
=======================================================
- Coverage   65.08%                64.77%   -0.31%     
+ Files          64                    66       +2     
+ Lines        9515                  9775     +260     
=======================================================
+ Hits         6192                  6331     +139     
+ Misses       3323                  3444     +121     
Files Changed Coverage
/primitives/traits/src/lib.rs 72.73% (-7.27%) 🔽

Coverage generated Fri Mar 22 14:15:48 UTC 2024

@tmpolaczyk tmpolaczyk marked this pull request as ready for review March 15, 2024 14:26
@girazoki girazoki requested a review from Agusrodri March 15, 2024 16:51
@girazoki
Copy link
Collaborator

Overall looks good. I think we are missing typescript tests as well for this

// They can be easily converted from one another, the difference is that absolute_multilocation
// has an extra "Parachain" junction in the front, using SelfParaId::get()
let interior_multilocation = Self::interior_multilocation(para_id);
let derived_account = Self::absolute_multilocation(interior_multilocation);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let derived_account = Self::absolute_multilocation(interior_multilocation);
let derived_account = Self::relay_relative_multilocation(interior_multilocation);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also rename interior_multilocation to something like local_relative_multilocation or local_interior_multilocation, to emphasize on the fact that the point of view of that multilocation is the local origin.

type GetBlockNumber: Get<u32>;
/// How to convert a `ParaId` into an `AccountId32`. Used to derive the parathread tank
/// account in `interior_multilocation`.
type GetParathreadAccountId: Convert<ParaId, [u8; 32]>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question. Who owns this account? Is it a chain identified by the corresponding para_id?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes exactly, it's a chain-derived account based on the paraId.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now this address is only usable for this purpose. However, in the future we might want the container-chain manager to have some power over this

pub struct BuyCoreCollatorProof<T: Config> {
account: T::AccountId,
// TODO
_signature: (),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signature is required, since we are spending tokens from the parathread tank account right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we decided not to make this PR bigger with the validation of the signature but @tmpolaczyk correct me if I am wrong. Also in that case let's update the TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the validation logic will be a separate PR so that it can be reviewed better

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me the current state of what has to be done in this PR is an approval. Missing pieces that should come as follow-ups are:

  • validation logic of the tx
  • slot checking (possibly with cooldown) within this pallet

Good job @tmpolaczyk

@tmpolaczyk tmpolaczyk merged commit ee30ff0 into master Mar 22, 2024
32 checks passed
@tmpolaczyk tmpolaczyk deleted the tomasz-xcm-coretime branch March 22, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants